Add TPU Ulysses context parallelism - #4687
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🤖 Hi @huytransformer, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This Pull Request introduces highly performant TPU Ulysses context parallelism on the TPU Tokamax Splash attention training path. The implementation is exceptionally clean, well-integrated, and backed by comprehensive unit, collective, and integration tests that verify correctness against dense references and compiled HLO collectives.
🔍 General Feedback
- Excellent Test Coverage: The newly added tests (including CPU multi-device mock checks and HLO collective pattern assertions) set a very high bar for correctness and robustness.
- Robust Config Validation: The comprehensive validation in
configs/types.pyensures that unsupported combinations (like MQA, packing, ragged attention, dropout, etc.) are proactively rejected with precise errors. - Clean Shared Utilities: Factoring out mesh and sharding operations into
context_parallel_utils.pyis a great design choice that improves maintainability. - Thorough Documentation: The update to
sharding.mdis incredibly detailed and accurately captures Ulysses constraints and architectural mechanics.
| if self.use_ragged_attention: | ||
| raise ValueError("TPU Ulysses attention does not support ragged attention.") | ||
| if self.config.context_parallel_load_balance: | ||
| raise ValueError("TPU Ulysses attention does not support context_parallel_load_balance.") |
There was a problem hiding this comment.
| raise ValueError("TPU Ulysses attention does not support context_parallel_load_balance.") | |
| if self.config.context_parallel_load_balance: | |
| raise ValueError("TPU Ulysses attention does not support context_parallel_load_balance=True.") |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
7d1fd35 to
e72bd0d
Compare
| from maxtext.kernels.attention import ulysses_attention | ||
|
|
||
|
|
||
| @pytest.mark.cpu_only |
There was a problem hiding this comment.
I think the default now is cpu_only
| from typing import Any | ||
|
|
||
| import jax | ||
|
|
There was a problem hiding this comment.
nit: I think all these three function are useful in general, we should consider moving them to sharding.py in the future
There was a problem hiding this comment.
Yeah, agree. It seems not context parallelism specific.
|
|
||
| def inverse_ulysses_all_to_all(tensor: Any, ulysses_axis: str): | ||
| """Moves `[B, H/U, S, D]` back to `[B, H, S/U, D]`.""" | ||
| return jax.lax.all_to_all(tensor, ulysses_axis, split_axis=2, concat_axis=1, tiled=True) |
There was a problem hiding this comment.
I love you using explicit all2all/shard_map instead of relying on XLA/auto_shard
|
|
||
| splash_kernel = wrap_ulysses_splash_kernel(mask) | ||
| segment_axis_names_splash_kernel = jax.sharding.PartitionSpec(None) | ||
| splash_kernel = self._maybe_shard_with_pspec(splash_kernel, segment_axis_names_splash_kernel) |
There was a problem hiding this comment.
why we want splash kernel been fully replicated? or is it an explicit all gather?
| query, key, value, decoder_segment_ids_tuple, sinks | ||
| ) | ||
| attention_output = ulysses_attention.inverse_ulysses_all_to_all(attention_output, context_axis) | ||
| return attention_output, None |
| f"close. ici_context_parallelism={ici_context_parallelism}.", | ||
| ) | ||
|
|
||
| @pytest.mark.tpu_only |
There was a problem hiding this comment.
if we just wanna check HLO, no need for TPU test. Use CPU test instead
| ulysses_attention.validate_ulysses_runtime(model_mode=MODEL_MODE_TRAIN, record_max_logits=True) | ||
|
|
||
| def test_with_sequence_axis_preserves_partition_spec_type(self): | ||
| spec = jax.sharding.PartitionSpec("data", None, None, "model") |
There was a problem hiding this comment.
we don't have 'model' axis name in maxtext?
| @@ -0,0 +1,93 @@ | |||
| # Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
this is a very interesting test
| @@ -0,0 +1,265 @@ | |||
| # Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
move this test to attention_test?
| @@ -0,0 +1,157 @@ | |||
| # Copyright 2026 Google LLC | |||
There was a problem hiding this comment.
move this test to attention test?
| mesh=mesh, | ||
| in_specs=P(None, None, "context", None), | ||
| out_specs=P(None, "context", None, None), | ||
| check_vma=False, |
There was a problem hiding this comment.
what gonna happen if we use check_vma=True?
RissyRan
left a comment
There was a problem hiding this comment.
Thanks! LGTM at high level. Some minor comments.
| "context_parallel_strategy='ring'." | ||
| ) | ||
| if context_parallel_strategy == "ulysses": | ||
| if self.hardware != "tpu": |
There was a problem hiding this comment.
Do you know why? If this feature hasn't been tested on GPUs yet, we should label it as 'experimental' and add a disclaimer that GPU execution is unverified and should be used with caution. Similar comments for bellow.
| raise ValueError("TPU Ulysses attention requires context_sharding='context'.") | ||
| ici_context_parallel_size = self.ici_context_parallelism | ||
| dcn_context_parallel_size = self.dcn_context_parallelism | ||
| if ici_context_parallel_size <= 0 or dcn_context_parallel_size <= 0: |
There was a problem hiding this comment.
MaxText won't support ici_context_parallel_size <= 0 or dcn_context_parallel_size <= 0 for other strategies right? This is invalid, not specific to this Ulysses. If so, we could just align with other sharding constratins.
| raise ValueError("TPU Ulysses attention does not support sparse indexer masks.") | ||
| if self.use_chunked_prefill: | ||
| raise ValueError("TPU Ulysses attention does not support chunked prefill yet.") | ||
| if self.use_multimodal: |
There was a problem hiding this comment.
Wondering if those features are not tested or needs feature to supported?
if self.use_multimodal:
raise ValueError("TPU Ulysses attention does not support multimodal attention.")
if self.enable_dropout and self.dropout_rate > 0.0:
raise ValueError("TPU Ulysses attention does not support dropout yet.")
...
| from typing import Any | ||
|
|
||
| import jax | ||
|
|
There was a problem hiding this comment.
Yeah, agree. It seems not context parallelism specific.
| dkv_dim_q=3, | ||
| dkv_dim_kv=3, | ||
| ) | ||
| if self.attention_kernel == "flash" and ulysses_attention.is_context_parallel_ulysses_requested(self.config): |
There was a problem hiding this comment.
It seems in this PR, we have quite some assertions, inside of types.py, those functions, etc. Do you think we could just keep one version in types.py for early check if no overwritten is allowed/happens during runtime.
| jax.shard_map, | ||
| mesh=mesh, | ||
| in_specs=( | ||
| P(None, None, "context", None), |
There was a problem hiding this comment.
It seems this test verified context sharding only. We usually will include fsdp + cp with long context. Could you help add a test and ensure those work (to avoid future change breakage)?
e72bd0d to
3cd54a9
Compare
Description
This PR adds
context_parallel_strategy="ulysses", DeepSpeed-Ulysses style context parallelism for the TPU splash attention (https://arxiv.org/abs/2309.14509).Motivations: (1) the existing approaches degrade when the context parallelism degree is large relative to the sequence length. (2) It is also the head-parallel half of a Ulysses x ring hybrid (USP), which can be built on top of this PR.
Performance
What we did: v5p-128 (64 chips), bf16, synthetic data, 2M tokens per step in every cell,
remat_policy=custom context=device,dq_reduction_steps=3. Each uses its best known block sizes (2048 for Ulysses, 1024 for Ring and All-gather).llama2-7b (MHA, 32 KV heads), 32K context, cp=16, 2K tokens per device.
Ulysses matches all-gather and runs 1.8x faster than ring, while holding 1/16 of all-gather's attention KV per device (2 of 32 heads).
llama3.1-8b (GQA, 8 KV heads), cp=4.
Correctness: 20 training steps against
all_gatheron llama3.1-8b at 64K, cp=4, identical data, max loss difference is 0.001.Example repro
Tests
tests/unit/ulysses_attention_test.py: Passed.tests/unit/ulysses_collective_test.py: Passed.tests/unit/configs_value_test.py: Passed.tests/integration/train_tests.py: train smoke test with ulysses cp=4. Passed.Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.